-
Notifications
You must be signed in to change notification settings - Fork 1.8k
out_s3: add support for server-side encryption headers #10901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Add configuration options to enable server-side encryption when uploading files to S3, supporting both AWS managed and customer managed KMS keys. This implementation follows AWS S3 documentation guidelines and adds support for the following encryption headers: * x-amz-server-side-encryption - Supported values: AES256, aws:kms, aws:kms:dsse * x-amz-server-side-encryption-aws-kms-key-id - Required when using customer managed KMS keys The feature allows users to encrypt their log data at rest in S3 buckets using their preferred encryption method. Reference: https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingServerSideEncryption.html Signed-off-by: Rishabh Sharma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direction of this patch is great but I found some issues to break our coding style.
So, please take a look on them?
| FLB_CONFIG_MAP_STR, "server-side-encryption", NULL, | ||
| 0, FLB_TRUE, offsetof(struct flb_s3, server_side_encryption), | ||
| "Server-side encryption type for S3 objects. Valid values: AES256, aws:kms, aws:kms:dsse. " | ||
| "When not specified, S3 applies the bucket's default encryption settings or SSE-S3 if no default is configured." | ||
| }, | ||
| { | ||
| FLB_CONFIG_MAP_STR, "server-side-encryption-aws-kms-key-id", NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use snake_case instead of kebab-case for parameter names.
|
|
||
| /* Validate sse values */ | ||
| if (ctx->server_side_encryption != NULL) { | ||
| if (strcmp(ctx->server_side_encryption, "AES256") != 0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We needn't to distinguish upper case or lower case.
So, we need to use strcasecmp instead of strcmp.
Plus, we need to verify with length, too.
On paper, it needs to use strncasecmp here.
| tmp = flb_output_get_property("server-side-encryption-aws-kms-key-id", ins); | ||
| if (tmp) { | ||
| ctx->server_side_encryption_aws_kms_key_id = (char *) tmp; | ||
| } | ||
|
|
||
| tmp = flb_output_get_property("server-side-encryption", ins); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After modifying to use camel_case on config_map, we need to fix using case_case here, too.
| flb_plg_error(ctx->ins, "server-side-encryption-aws-kms-key-id requires server-side-encryption to be 'aws:kms' or 'aws:kms:dsse'"); | ||
| return -1; | ||
| } | ||
| if (strcmp(ctx->server_side_encryption, "aws:kms") != 0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here for being needed to use strncasecmp.
Add configuration options to enable server-side encryption when uploading files to S3, supporting both AWS managed and customer managed KMS keys.
This implementation follows AWS S3 documentation guidelines and adds support for the following encryption headers:
The feature allows users to encrypt their log data at rest in S3 buckets using their preferred encryption method.
Reference: https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingServerSideEncryption.html
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Masking s3 bucket name and kms key arn in the logs
valid_no_sse.conf
valid_sse_aes256.conf
valid_sse_kms.conf
valid_sse_kms_dsse.conf
valid_sse_kms_with_key.conf
valid_sse_kms_dsse_with_key.conf
invalid_sse_value.conf
kms_key_without_sse.conf
kms_key_with_aes256.conf
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.